router-tests/structured_logging_test.go (1)

712-712: Consistent migration to object-based retry options in tests

All updated WithSubgraphRetryOptions(core.NewSubgraphRetryOptions(... Enabled: false ...)) calls look correct and keep logs deterministic by avoiding subgraph retries.

If you find yourself disabling retries across many tests, consider a small helper in testenv (e.g., testenv.DisableSubgraphRetriesOption()) to reduce repetition and ease future changes.

Also applies to: 832-832, 964-964, 1100-1100, 2214-2214

router-tests/telemetry/telemetry_test.go (2)

8695-8700: Per-subgraph retry builder usage looks correct; optionally assert “no retries” explicitly

Switching to core.WithSubgraphRetryOptions(core.NewSubgraphRetryOptions(...)) with Enabled: false matches the new API and aligns with our prior learning that algorithms shouldn’t be validated when retries are disabled. If you want a stronger assertion, consider wiring an OnRetryFunc (if exposed on SubgraphRetryOptions) to increment a counter and assert it remains zero during this test.

---

9639-9642: Check all datapoints, not just the first, for absence of the subgraph attribute

Guard against future exporter changes that may add more datapoints by asserting across the full set.

-					data2 := httpRequestsMetric.Data.(metricdata.Sum[int64])
-					atts := data2.DataPoints[0].Attributes
-					_, ok := atts.Value(attribute.Key(key))
-					require.False(t, ok)
+					data2 := httpRequestsMetric.Data.(metricdata.Sum[int64])
+					for _, dp := range data2.DataPoints {
+						_, ok := dp.Attributes.Value(attribute.Key(key))
+						require.False(t, ok)
+					}

router-tests/error_handling_test.go (1)

1383-1387: Good migration to manager-based retry config; consider a helper to DRY tests

The switch to core.WithSubgraphRetryOptions(core.NewSubgraphRetryOptions(...)) with Enabled:false is correct and matches our prior learning that disabled retries shouldn’t validate algorithm/fields. You repeat the same disabled config 4x; a tiny helper keeps tests concise.

Apply this to replace the repeated blocks:

-                core.WithSubgraphRetryOptions(core.NewSubgraphRetryOptions(config.TrafficShapingRules{
-                    All: config.GlobalSubgraphRequestRule{
-                        BackoffJitterRetry: config.BackoffJitterRetry{Enabled: false},
-                    },
-                })),
+                disabledSubgraphRetries(),

Add this helper near the top of the file (outside the selected ranges):

// test helper
func disabledSubgraphRetries() core.Option {
	return core.WithSubgraphRetryOptions(core.NewSubgraphRetryOptions(config.TrafficShapingRules{
		All: config.GlobalSubgraphRequestRule{
			BackoffJitterRetry: config.BackoffJitterRetry{Enabled: false},
		},
	}))
}

Also applies to: 1419-1423, 1455-1459, 1491-1495

router/core/graph_server.go (1)

266-287: router/core/graph_server.go: guard nil retryOptions and wrap Initialize error

-    if s.retryOptions.IsEnabled() {
+    if s.retryOptions != nil && s.retryOptions.IsEnabled() {
         retryExprManager := expr.NewRetryExpressionManager()
         // …
         err = retryManager.Initialize(
             s.retryOptions.All,
             s.retryOptions.SubgraphMap,
             routerConfig.GetSubgraphs(),
         )
-        if err != nil {
-            return nil, err
-        }
+        if err != nil {
+            return nil, fmt.Errorf("failed to initialize retry manager: %w", err)
+        }
         s.retryManager = retryManager
     }

router/internal/expr/retry_expression.go (3)

16-23: Constructor is fine; optional: precompile default expression

As a small UX improvement, consider precompiling the default expression during construction so first evaluations don’t depend on prior AddExpression calls.

Example (if you decide to change signature elsewhere to allow error return):

m := &RetryExpressionManager{expressionMap: make(map[string]*vm.Program)}
_ = m.AddExpression("") // ignore error; Initialize can still validate real strings
return m

---

25-50: Normalize expression key or fix the comment

Comment says “normalized” but no normalization occurs. Either trim/normalize or update the comment.

 import (
     "fmt"
     "reflect"
+    "strings"

     "github.com/expr-lang/expr"
     "github.com/expr-lang/expr/vm"
 )
@@
-func (c *RetryExpressionManager) AddExpression(exprString string) error {
-    expression := exprString
+func (c *RetryExpressionManager) AddExpression(exprString string) error {
+    expression := strings.TrimSpace(exprString)
@@
-    // Use the normalized expression string as the key for deduplication
+    // Use the trimmed expression string as the key for deduplication
     c.expressionMap[expression] = program
     return nil
 }

---

53-80: Consider lazy compilation on cache miss instead of silently returning false

Fail-closed is safe, but lazily compiling unknown expressions can reduce surprises if Initialize missed one.

 func (m *RetryExpressionManager) ShouldRetry(ctx RetryContext, expressionString string) (bool, error) {
     if m == nil {
         return false, nil
     }

-    expression := expressionString
+    expression := expressionString
     if expression == "" {
         expression = defaultRetryExpression
     }

     program, ok := m.expressionMap[expression]
     if !ok {
-        // If the expression wasn't pre-compiled, do not retry by default
-        return false, nil
+        // Lazily compile the expression once
+        if err := m.AddExpression(expression); err != nil {
+            return false, err
+        }
+        program = m.expressionMap[expression]
     }

If AddExpression can be invoked concurrently in your environment, add an RWMutex around expressionMap accesses.

Would you like me to add a minimal, lock-guarded version?

router/internal/expr/retry_expression_test.go (2)

141-146: Strengthen the empty-expression test

Consider asserting runtime behavior as well (e.g., ShouldRetry returns false) to guard against regressions, not just that AddExpression("") succeeds and the manager is non-nil.

 err := manager.AddExpression("")
 assert.NoError(t, err)
 assert.NotNil(t, manager)
+res, evalErr := manager.ShouldRetry(RetryContext{}, "")
+assert.NoError(t, evalErr)
+assert.False(t, res)

---

528-551: Parallelize subtests to speed up and isolate failures (optional)

You can mark the top-level test and subtests with t.Parallel() for faster runs; table-driven subtests here are good candidates.

 func TestRetryExpressionManager_WithSyscallErrors(t *testing.T) {
+  t.Parallel()
   t.Run("expression combining status and syscall errors", func(t *testing.T) {
+    t.Parallel()

router/core/router.go (3)

187-206: SubgraphRetryOptions: shape and IsEnabled() look good

Structure and enablement logic are sound; nil map iteration is safe. Minor: consider doc comments on fields for clarity (global “All” vs per-subgraph overrides).

---

1908-1923: Comment mismatch: “circuit breakers” → “retry options”

The comment under NewSubgraphRetryOptions mentions circuit breakers; it should refer to retry options.

- // Subgraph specific circuit breakers
+ // Subgraph-specific retry options

---

1924-1937: Default algorithm only when retries are enabled (aligns with team preference)

Per team learning, when retries are disabled we shouldn’t “enforce” algorithm defaults. Guard the defaulting by Enabled to better reflect that stance.

-func newRetryConfig(config config.BackoffJitterRetry) retrytransport.RetryOptions {
-    algorithm := config.Algorithm
-    if algorithm == "" {
-        algorithm = retrytransport.BackoffJitter
-    }
+func newRetryConfig(config config.BackoffJitterRetry) retrytransport.RetryOptions {
+    algorithm := config.Algorithm
+    if config.Enabled && algorithm == "" {
+        algorithm = retrytransport.BackoffJitter
+    }
     return retrytransport.RetryOptions{
         Enabled:       config.Enabled,
         Algorithm:     algorithm,
         MaxRetryCount: config.MaxAttempts,
         MaxDuration:   config.MaxDuration,
         Interval:      config.Interval,
         Expression:    config.Expression,
     }
 }

router/core/retry_builder.go (1)

15-23: Defensive nil check for req (optional)

If ShouldRetry ever gets a nil req from the transport layer, req.Context() will panic. Cheap guard:

-reqContext := getRequestContext(req.Context())
+if req == nil {
+  return false
+}
+reqContext := getRequestContext(req.Context())

router-tests/retry_test.go (1)

76-78: Avoid brittle full-JSON equality in assertions.

These assertions will break on harmless message changes. Prefer checking essential fields (e.g., statusCode) or comparing decoded JSON structures.
Example:

- require.Equal(t, `{"errors":[{"message":"Failed to fetch from Subgraph 'employees', Reason: empty response.","extensions":{"statusCode":502}}],"data":{"employees":null}}`, res.Body)
+ require.JSONEq(t, `{"data":{"employees":null}}`, res.Body)
+ require.Contains(t, res.Body, `"statusCode":502`)

Also applies to: 127-129, 180-182, 296-298, 360-372

router/internal/retrytransport/retry_transport_test.go (2)

268-298: Add a zero-retries guard test.

Add a case with MaxRetryCount=0 to assert no retries occur and OnRetry isn’t called.

Sketch:

t.Run("no retries when MaxRetryCount=0", func(t *testing.T) {
  retries, attempts := 0, 0
  mgr := newTestManager(simpleShouldRetry, func(int, *http.Request, *http.Response, time.Duration, error) { retries++ },
    RetryOptions{Enabled: true, MaxRetryCount: 0, Interval: time.Millisecond, MaxDuration: time.Second}, "sg")
  tr := NewRetryHTTPTransport(&MockTransport{handler: func(*http.Request) (*http.Response, error) {
    attempts++; return nil, errors.New("fail")
  }}, func(*http.Request) *zap.Logger { return zap.NewNop() }, mgr)
  _, err := tr.RoundTrip(withSubgraph(httptest.NewRequest("GET", "http://x", nil), "sg"))
  assert.Error(t, err)
  assert.Equal(t, 0, retries)
  assert.Equal(t, 1, attempts)
})

---

383-414: Log-string assertions are inherently brittle.

If wording changes, tests fail. Consider matching on a stable key/field or use zap observer to assert fields instead of formatted strings.

router/core/retry_builder_test.go (4)

52-57: Test name doesn’t match behavior.

No “disabled” path is exercised here; the test only builds the function. Rename to “build function returns non-nil with empty manager” or add a real disabled-case.

-t.Run("build function when retry is disabled", func(t *testing.T) {
+t.Run("build function returns non-nil with empty manager", func(t *testing.T) {

---

176-189: Clarify EOF comment.

Comment says “EOF is now handled at transport layer,” but this test expects true here via default retryable error handling. Tweak the comment to avoid confusion.

- // EOF is now handled at transport layer, not expression
+ // EOF can be retried via default transport-level detection; expression is false here

---

442-485: Remove stale, commented-out tests.

Dead code makes maintenance harder. Either port to the new manager-based API or delete.

-// TODO: Fix
-//func TestProcessRetryOptions(t *testing.T) {
-//  ... (entire commented block)
-//}
+// (removed legacy ProcessRetryOptions tests; superseded by expression-manager and manager-based flows)

---

51-57: Add a “no request context” guard test.

BuildRetryFunction returns false when request context is missing. Add a unit test to lock this in.

Sketch:

t.Run("returns false when request context missing", func(t *testing.T) {
  manager := expr.NewRetryExpressionManager()
  _ = manager.AddExpression("true")
  fn, _ := BuildRetryFunction(manager)
  req, _ := http.NewRequest("POST", "http://example.com/graphql", nil) // no ctx attached
  assert.False(t, fn(nil, req, &http.Response{StatusCode: 500}, "true"))
})

Want me to open a follow-up PR adding this?

Also applies to: 139-146

router/internal/retrytransport/manager.go (2)

88-91: Remove unreachable config-not-found branch.

customSgNames is only populated when a subgraph has an entry in subgraphRetryOptions; this branch cannot trigger.

-		entry, ok := subgraphRetryOptions[sgName]
-		if !ok {
-			joinErr = errors.Join(joinErr, errors.New("retry config not found for subgraph "+sgName))
-			continue
-		}
+		entry := subgraphRetryOptions[sgName]

---

118-121: Rename local variable for clarity.

Avoid “circuitBreaker” in retry code.

-	if circuitBreaker, ok := c.retries[name]; ok {
-		return circuitBreaker
+	if opts, ok := c.retries[name]; ok {
+		return opts

router/internal/retrytransport/retry_transport.go (2)

145-151: Option: drain before sleeping to maximize connection reuse.

Draining immediately frees the connection for reuse while we wait.

-		// drain the previous response before retrying
-		rt.drainBody(resp, requestLogger)
-
-		// Retry the request
-		resp, err = rt.roundTripper.RoundTrip(req)
+		// drain the previous response before waiting/retrying
+		rt.drainBody(resp, requestLogger)
+		// Retry the request
+		resp, err = rt.roundTripper.RoundTrip(req)

---

50-56: Log level nit: malformed Retry-After.

Consider Warn instead of Error to reduce noise for client-driven header issues.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: SkArchon
PR: wundergraph/cosmo#2167
File: router-tests/structured_logging_test.go:712-713
Timestamp: 2025-08-28T10:07:27.637Z
Learning: In the router retry configuration, when retries are disabled (enabled=false in WithSubgraphRetryOptions), the algorithm parameter should not be validated since it won't be used. Empty algorithm strings are acceptable when retries are disabled.

Learnt from: endigma
PR: wundergraph/cosmo#2155
File: router/core/router.go:1857-1866
Timestamp: 2025-08-20T10:08:17.857Z
Learning: router/pkg/config/config.schema.json forbids null values for traffic_shaping.subgraphs: additionalProperties references $defs.traffic_shaping_subgraph_request_rule with type "object". Therefore, in core.NewSubgraphTransportOptions, dereferencing each subgraph rule pointer is safe under schema-validated configs, and a nil-check is unnecessary.

Learnt from: SkArchon
PR: wundergraph/cosmo#2167
File: router-tests/structured_logging_test.go:712-713
Timestamp: 2025-08-28T10:07:27.637Z
Learning: In the router retry configuration, when retries are disabled (enabled=false in WithSubgraphRetryOptions), the algorithm parameter should not be validated since it won't be used. Empty algorithm strings are acceptable when retries are disabled.

Learnt from: endigma
PR: wundergraph/cosmo#2155
File: router/core/router.go:1857-1866
Timestamp: 2025-08-20T10:08:17.857Z
Learning: In the Cosmo router codebase, JSON schema validation prevents null values in TrafficShapingRules subgraph configurations, making nil checks unnecessary when dereferencing subgraph rule pointers in NewSubgraphTransportOptions.

Learnt from: SkArchon
PR: wundergraph/cosmo#2137
File: router/pkg/pubsub/redis/adapter.go:114-116
Timestamp: 2025-08-14T16:47:03.744Z
Learning: In the Cosmo router codebase, EventMetricStore fields are never nil - the system uses the null object pattern with NewNoopEventMetricStore() when metrics are disabled, eliminating the need for nil checks before calling EventMetricStore methods.

Learnt from: SkArchon
PR: wundergraph/cosmo#2167
File: router/core/retry_expression_builder.go:3-11
Timestamp: 2025-08-26T17:30:41.212Z
Learning: In router/core/retry_expression_builder.go, the team prefers to keep EOF error detection simple with string matching ("unexpected eof") rather than using errors.Is() for io.EOF and io.ErrUnexpectedEOF. They want to wait for customer feedback before making it more robust, since customers can modify retry expressions to handle specific cases.

Learnt from: SkArchon
PR: wundergraph/cosmo#2167
File: router/core/supervisor_instance.go:196-204
Timestamp: 2025-08-28T09:59:19.956Z
Learning: In router/pkg/config/config.go, the BackoffJitterRetry struct uses envDefault tags to provide default values for Algorithm ("backoff_jitter") and Expression ("IsRetryableStatusCode() || IsConnectionError() || IsTimeout()"), ensuring that even when YAML configs omit these fields, valid defaults are applied automatically, preventing ProcessRetryOptions from rejecting empty values.

Learnt from: SkArchon
PR: wundergraph/cosmo#2167
File: router/internal/expr/retry_context.go:3-10
Timestamp: 2025-08-26T17:19:28.178Z
Learning: In the Cosmo router codebase, prefer using netErr.Timeout() checks over explicit context.DeadlineExceeded checks for timeout detection in retry logic, as Go's networking stack typically wraps context deadline exceeded errors in proper net.Error implementations.

router-tests/retry_test.go (3)

503-531: Great: base-level invalid algorithm is correctly rejected at init.
Matches expected behavior and error text.

---

533-558: Nice: invalid algorithm is ignored when retries are disabled (base).
This addresses the previously requested coverage.

---

560-587: Nice: per-subgraph “disabled ignores algorithm” case covered.
Prevents regressions in validation gating.

router-tests/retry_test.go (8)

38-51: DRY the options setup with a small helper to reduce duplication.
Multiple tests repeat the same NewSubgraphRetryOptions + WithSubgraphRetryOptions pattern; a local builder will simplify maintenance.

Apply this helper once near the top and replace repeated blocks:

+func withRetryOpts(cfg config.TrafficShapingRules, onRetry func(int, *http.Request, *http.Response, time.Duration, error)) core.Option {
+  opts := core.NewSubgraphRetryOptions(cfg)
+  opts.OnRetryFunc = onRetry
+  return core.WithSubgraphRetryOptions(opts)
+}

Also applies to: 92-105, 145-158, 199-212, 260-273, 324-337, 391-404, 451-464

---

224-231: Decouple success trigger from OnRetry callback semantics.
Using onRetryCounter to flip success couples the test to callback timing. Drive success via request attempt count to avoid false positives if OnRetry invocation timing changes.

- if onRetryCounter.Load() == int32(maxAttemptsBeforeServiceSucceeds) {
+ if serviceCallsCounter.Load() == int32(maxAttemptsBeforeServiceSucceeds) {
    w.WriteHeader(http.StatusOK)
    _, _ = w.Write([]byte(`{"data":{"employees":[{"id":1},{"id":2}]}}`))
  } else {
    w.WriteHeader(http.StatusBadGateway)
  }
  serviceCallsCounter.Add(1)

---

256-257: Speed up the 429 + Retry-After test.
This test currently sleeps ~3s (3 retries × 1s). One retry is sufficient to validate header precedence.

- maxRetryCount := 3
+ maxRetryCount := 1

---

433-434: Avoid flakiness: assert a range instead of NotEqual.
Exact equality with retryInterval can sporadically occur depending on jitter/rounding.

- require.NotEqual(t, sleepDuration.Load(), int64(retryInterval))
+ d := time.Duration(sleepDuration.Load())
+ require.Greater(t, d, time.Duration(0))
+ require.Less(t, d, 2*retryInterval)

---

494-495: Same here: use bounded checks to reduce flakiness.
Zero Retry-After should fall back to jitter/backoff; prefer range assertions.

- require.NotEqual(t, sleepDuration.Load(), int64(retryInterval))
+ d := time.Duration(sleepDuration.Load())
+ require.Greater(t, d, time.Duration(0))
+ require.Less(t, d, 2*retryInterval)

---

360-362: Prefer JSON structural equality over string equality for response bodies.
String comparisons are brittle to formatting/ordering changes.

- require.Equal(t, `{"errors":[{"message":"Failed to fetch from Subgraph 'employees', Reason: empty response.","extensions":{"statusCode":502}}],"data":{"employees":null}}`, res.Body)
+ require.JSONEq(t, `{"errors":[{"message":"Failed to fetch from Subgraph 'employees', Reason: empty response.","extensions":{"statusCode":502}}],"data":{"employees":null}}`, res.Body)

(Apply similarly to the other exact body checks.)

Also applies to: 681-685, 761-763

---

366-372: Widen timing slack to reduce CI flakes.
A 20ms/100ms margin can be tight under load; slightly larger buffers help.

- shouldBeLessThanDuration := (time.Duration(maxRetryCount-1) * retryInterval) - (20 * time.Millisecond)
+ shouldBeLessThanDuration := (time.Duration(maxRetryCount-1) * retryInterval) - (50 * time.Millisecond)
...
- expectedMinDuration := (time.Duration(maxRetryCount-1) * maxDuration) - (100 * time.Millisecond)
+ expectedMinDuration := (time.Duration(maxRetryCount-1) * maxDuration) - (150 * time.Millisecond)

---

621-697: Per-subgraph retry counts validated correctly.
Isolated counters for employees vs test1 look good.

If useful, I can add a combined query hitting both subgraphs in a single operation to verify independent retry accounting within one request.

router/internal/retrytransport/retry_transport.go (6)

142-144: Honor context cancellation/deadlines during backoff sleep.

Avoid sleeping past ctx deadlines; exit early if canceled.

-		// Wait for the specified duration
-		time.Sleep(sleepDuration)
+		// Wait for the specified duration or context cancellation
+		if sleepDuration > 0 {
+			select {
+			case <-req.Context().Done():
+				return resp, req.Context().Err()
+			case <-time.After(sleepDuration):
+			}
+		}

---

59-66: Also honor Retry-After for 503 Service Unavailable.

RFC 7231 allows Retry-After on 503 responses as well as 429. This improves interoperability with upstreams.

-	if resp == nil || resp.StatusCode != http.StatusTooManyRequests {
+	if resp == nil || (resp.StatusCode != http.StatusTooManyRequests && resp.StatusCode != http.StatusServiceUnavailable) {
 		return 0, false
 	}

---

124-128: Generic log for Retry-After usage (it may be 429 or 503).

-			requestLogger.Debug("Using Retry-After header for 429 response",
+			requestLogger.Debug("Using Retry-After header",
 				zap.Int("retry", retries),
 				zap.String("url", req.URL.String()),
 				zap.Duration("retry-after", sleepDuration),
 			)

---

51-53: Lower log level for malformed Retry-After headers.

Header parsing errors are client-observable but non-fatal; debug (or warn) is less noisy in production.

-		logger.Error("Failed to parse Retry-After header", zap.String("retry-after", retryAfter), zap.Error(errJoin))
+		logger.Debug("Failed to parse Retry-After header", zap.String("retry-after", retryAfter), zap.Error(errJoin))

---

173-176: Tighten the comment wording.

-	// When we close the body only will go internally marks the persisted connection as true
-	// which is important so that it can reuse the connection internally for retrying
+	// Draining then closing marks the connection as reusable, allowing the client to reuse it for retries.

---

105-111: Optional: short-circuit when retries are effectively disabled.

If MaxRetryCount == 0 (or options are disabled), return immediately to avoid backoff setup and logging overhead. Note: per team learning, IsEnabled() handles nil receivers safely if available.

 	retryOptions := rt.retryManager.GetSubgraphOptions(subgraph)
-	if retryOptions == nil {
-		return resp, nil
-	}
+	if retryOptions == nil {
+		return resp, err
+	}
+	// Optional fast-exit:
+	// if retryOptions.MaxRetryCount == 0 { return resp, err }
+	// or if available:
+	// if !retryOptions.IsEnabled() { return resp, err }

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: SkArchon
PR: wundergraph/cosmo#2172
File: router/core/router_config.go:221-221
Timestamp: 2025-09-01T13:44:13.033Z
Learning: The IsEnabled() method on SubgraphRetryOptions in router/core/router.go safely handles nil receivers by returning false as the first check, so no external nil checks are needed when calling c.retryOptions.IsEnabled(). This is a consistent pattern used across multiple IsEnabled() methods in the codebase.

Learnt from: endigma
PR: wundergraph/cosmo#2155
File: router/core/router.go:1857-1866
Timestamp: 2025-08-20T10:08:17.857Z
Learning: router/pkg/config/config.schema.json forbids null values for traffic_shaping.subgraphs: additionalProperties references $defs.traffic_shaping_subgraph_request_rule with type "object". Therefore, in core.NewSubgraphTransportOptions, dereferencing each subgraph rule pointer is safe under schema-validated configs, and a nil-check is unnecessary.

Learnt from: SkArchon
PR: wundergraph/cosmo#2172
File: router/core/router_config.go:221-221
Timestamp: 2025-09-01T13:44:13.033Z
Learning: The IsEnabled() method on SubgraphRetryOptions in router/core/router.go safely handles nil receivers by returning false, so no external nil checks are needed when calling c.retryOptions.IsEnabled().

Learnt from: SkArchon
PR: wundergraph/cosmo#2167
File: router-tests/structured_logging_test.go:712-713
Timestamp: 2025-08-28T10:07:27.637Z
Learning: In the router retry configuration, when retries are disabled (enabled=false in WithSubgraphRetryOptions), the algorithm parameter should not be validated since it won't be used. Empty algorithm strings are acceptable when retries are disabled.

router/internal/retrytransport/retry_transport.go (1)

104-108: Nice fix: avoids a second request when no retry options

Returning the outcome of the first attempt preserves semantics and errors.

router/internal/retrytransport/retry_transport.go (5)

58-75: Honor Retry-After for 503 as well and accept zero-second delays

RFC 7231 defines Retry-After for 503 and 429. Also treat “0” as a valid immediate retry (no sleep), not a fallback to random backoff.

Apply:

-func shouldUseRetryAfter(logger *zap.Logger, resp *http.Response, maxDuration time.Duration) (time.Duration, bool) {
-	if resp == nil || resp.StatusCode != http.StatusTooManyRequests {
+func shouldUseRetryAfter(logger *zap.Logger, resp *http.Response, maxDuration time.Duration) (time.Duration, bool) {
+	if resp == nil || (resp.StatusCode != http.StatusTooManyRequests && resp.StatusCode != http.StatusServiceUnavailable) {
 		return 0, false
 	}
@@
-	return duration, duration > 0
+	// Accept immediate retry (0s) as valid
+	return duration, duration >= 0

---

22-56: Minor: trim header value before parsing; lower log level on parse failures

Real-world proxies sometimes add whitespace; parsing “ 120 ” should work. Also consider debug level to reduce noise.

Apply:

 import (
 	"errors"
 	"io"
 	"net/http"
+	"strings"
 	"strconv"
 	"time"
@@
-func parseRetryAfterHeader(logger *zap.Logger, retryAfter string) time.Duration {
+func parseRetryAfterHeader(logger *zap.Logger, retryAfter string) time.Duration {
 	if retryAfter == "" {
 		return 0
 	}
 
 	var errJoin error
 
-	seconds, err := strconv.Atoi(retryAfter)
+	retryAfter = strings.TrimSpace(retryAfter)
+	seconds, err := strconv.Atoi(retryAfter)
@@
-	if errJoin != nil {
-		logger.Error("Failed to parse Retry-After header", zap.String("retry-after", retryAfter), zap.Error(errJoin))
-	}
+	if errJoin != nil {
+		logger.Debug("Failed to parse Retry-After header", zap.String("retry-after", retryAfter), zap.Error(errJoin))
+	}

---

144-149: Drain before sleeping to free connections earlier

Draining before the wait reduces connection pinning during long sleeps.

(Already included in the diff above.)

---

183-187: Guard against nil resp in helpers (defensive)

Not strictly necessary given current call sites, but adding a nil check makes the helper safer.

Apply:

 func isResponseOK(resp *http.Response) bool {
+	if resp == nil {
+		return false
+	}
 	// Ensure we don't wait for no reason when subgraphs don't behave
 	// spec-compliant and returns a different status code than 200.
 	return resp.StatusCode >= 200 && resp.StatusCode < 300
 }

---

175-181: Polish the comment grammar

Improve clarity of the connection reuse note.

Apply:

-	// When we close the body only will go internally marks the persisted connection as true
-	// which is important so that it can reuse the connection internally for retrying
+	// Fully consuming and closing the body marks the connection reusable
+	// so the Transport can keep-alive and reuse it for retries.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: SkArchon
PR: wundergraph/cosmo#2167
File: router-tests/structured_logging_test.go:712-713
Timestamp: 2025-08-28T10:07:27.637Z
Learning: In the router retry configuration, when retries are disabled (enabled=false in WithSubgraphRetryOptions), the algorithm parameter should not be validated since it won't be used. Empty algorithm strings are acceptable when retries are disabled.

Learnt from: SkArchon
PR: wundergraph/cosmo#2172
File: router/core/router_config.go:221-221
Timestamp: 2025-09-01T13:44:13.033Z
Learning: The IsEnabled() method on SubgraphRetryOptions in router/core/router.go safely handles nil receivers by returning false as the first check, so no external nil checks are needed when calling c.retryOptions.IsEnabled(). This is a consistent pattern used across multiple IsEnabled() methods in the codebase.

Learnt from: SkArchon
PR: wundergraph/cosmo#2172
File: router/core/router_config.go:221-221
Timestamp: 2025-09-01T13:44:13.033Z
Learning: The IsEnabled() method on SubgraphRetryOptions in router/core/router.go safely handles nil receivers by returning false, so no external nil checks are needed when calling c.retryOptions.IsEnabled().

Learnt from: SkArchon
PR: wundergraph/cosmo#2172
File: router/core/retry_builder.go:13-15
Timestamp: 2025-09-01T13:26:16.245Z
Learning: In router/core/retry_builder.go, the BuildRetryFunction's manager parameter is guaranteed to be non-nil by the calling code, so defensive nil checks are not needed according to the team's preference.

Learnt from: endigma
PR: wundergraph/cosmo#2155
File: router/core/router.go:1857-1866
Timestamp: 2025-08-20T10:08:17.857Z
Learning: router/pkg/config/config.schema.json forbids null values for traffic_shaping.subgraphs: additionalProperties references $defs.traffic_shaping_subgraph_request_rule with type "object". Therefore, in core.NewSubgraphTransportOptions, dereferencing each subgraph rule pointer is safe under schema-validated configs, and a nil-check is unnecessary.